Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[해나] 회원가입 미션 1, 2단계 제출합니다. #15

Open
wants to merge 9 commits into
base: hxeyexn
Choose a base branch
from

Conversation

hxeyexn
Copy link

@hxeyexn hxeyexn commented Oct 11, 2024

안녕하세요 오둥이! 오랜만이네요 🥹
Compose는 처음이라 어렵네요 ㅎㅎ
리뷰 잘 부탁드리겠습니다!!

시연영상

default.webm

Composable 함수를 분리하는 기준

Composable 함수를 분리하는 기준을 고민하다가 아래와 같이 xml에서 style을 정의하는 기준으로 분리해보려 했습니다!
Composable 함수 분리도 재활용을 위한 거라고 생각해서 style과 비슷하다는 생각이 들었어요!!

    <style name="ButtonStyle">
        <item name="android:layout_width">0dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:minWidth">0dp</item>
        <item name="android:minHeight">0dp</item>
        <item name="android:paddingVertical">14dp</item>
        <item name="android:stateListAnimator">@null</item>
        <item name="android:textAppearance">@style/Typography.Title3</item>
    </style>

오둥이는 어떤 기준으로 분리했는지도 궁금하네요!

@hxeyexn hxeyexn requested a review from murjune October 11, 2024 06:36
@hxeyexn hxeyexn self-assigned this Oct 11, 2024
@hxeyexn hxeyexn changed the base branch from main to hxeyexn October 11, 2024 06:37
Copy link

@murjune murjune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 해나! 1, 2 단계 미션 요구사항을 빠르게 잘 구현해주셨네요 👏
해나와 함께 고민해봤으면 좋겠는 점을 코멘트로 남겼으니 곰곰히 생각하고 답글 달아주세요~!~!

@@ -28,3 +29,4 @@ androidx-material3 = { group = "androidx.compose.material3", name = "material3"
[plugins]
android-application = { id = "com.android.application", version.ref = "agp" }
jetbrains-kotlin-android = { id = "org.jetbrains.kotlin.android", version.ref = "kotlin" }
ktlint = { id = "org.jlleitschuh.gradle.ktlint", version.ref = "ktlint" }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ktLint도 하셨네요 💯 (난 안했는데..)

@@ -1,5 +1,5 @@
[versions]
agp = "8.5.1"
agp = "8.3.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agp 를 다운시킨 이유가 있으실까요!

}

@Composable
fun ButtonView(
Copy link

@murjune murjune Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View 와 Compose 를 동시에 사용하는 프로젝트에서 컴포저블 함수가 xxxView 라는 네이밍이라면 헷갈릴 것 같아요!
따라서, 저는 기존 전통 View 시스템과의 구분을 위해 Compose 에서는 View 라는 일반적으로 postfix 를 사용하지 않습니다.

Modifier
.fillMaxSize()
.padding(top = 56.dp, start = 32.dp, end = 32.dp),
color = MaterialTheme.colorScheme.background,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surface 는 기본적으로 MaterialTheme.colorScheme.surface 로 지정되어있습니다!

MaterialTheme.colorScheme.background 로 color 를 바꿔주신 이유가 따로 있으실까요??

import nextstep.signup.ui.theme.Blue50

@Composable
fun TextView(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마찬가지로 XML TextView 와 헷갈릴 수 있을 것 같습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 StringRes 보다는 String 으로 값을 넘기는걸 선호합니다!
StringRes 의 경우 정적이기 때문에 동적으로 String 이 변환돼서 온다면 이를 해결할 방법이 없기 때문입니다.

예시를 들어볼게요!
만약, ViewModel 에서 userName을 보내주고 TextView 에서 userName를 보여주고 싶다고 해봅시다. 그런데 description 가 StringRes 라 표시해줄 방법이 없겠네요 😢

TextView 의 경우 더더욱 기본 컴포넌트이기에 다른 컴포넌트에 비해 훨씬 유연성이 높아야한다고 생각해요! 따라서, String 타입으로 변경하는걸 제안드립니다 💪

@StringRes label: Int,
keyboardType: KeyboardType = KeyboardType.Text,
) {
var input by remember { mutableStateOf("") }
Copy link

@murjune murjune Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TextView 가 직접 상태를 관리하는 대신 부모로 상태를 올려볼까요? 이를 상태 호이스팅기법이라 합니다.

질문1) 왜 상태 호이스팅을 해야할까요??
질문2) StateLess 컴포저블과 StateFull 컴포저블의 차이는 뭘까요? 그리고 어떤 차이가 있을까요?

힌트) 만약 유저 정보를 사용자에게 입력받아 서버로 보내려면 어떻게 해야될까요? 🤔

Comment on lines +51 to +52
TextFieldView(paddingTop = 36.dp, label = R.string.main_email, keyboardType = KeyboardType.Email)
TextFieldView(paddingTop = 36.dp, label = R.string.main_password, keyboardType = KeyboardType.Password)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KeyboradType 을 잘 바꿔주셨네요. 저는 Email 타입도 지정 안했는데 덕분에 알아갑니다 👍👍👍

TextFieldView(paddingTop = 42.dp, label = R.string.main_user_name)
TextFieldView(paddingTop = 36.dp, label = R.string.main_email, keyboardType = KeyboardType.Email)
TextFieldView(paddingTop = 36.dp, label = R.string.main_password, keyboardType = KeyboardType.Password)
TextFieldView(paddingTop = 36.dp, label = R.string.main_password_confirm, keyboardType = KeyboardType.Password)
}

@Preview(showBackground = true)
@Composable
fun GreetingPreview() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preview 의 경우 private 키워드 붙이는 걸 제안드립니다! 다른 파일에서 해당 Preview를 참조할 수 있으면 안되겠죠? 😉

Suggested change
fun GreetingPreview() {
private fun SignUpScreenPreview() {

Comment on lines +38 to +40
TextView(R.string.main_greeting)
TextFieldScreen()
ButtonView(R.string.main_sign_up, paddingTop = 42.dp)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보통 Screen 은 화면 단위에 붙이는 네이밍이에요! 그러나 현재 Fragment? 같은 느낌으로 사용되고 있네요

화면 단위로 리팩토링해볼까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고, TextFieldScreen 보다는 SignUpScreen 이란 네이밍은 어떠신가요!

import org.junit.Rule
import org.junit.Test

class LayoutBasicsTest {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1단계를 완벽하게 수행해주셨네요!

@murjune
Copy link

murjune commented Oct 11, 2024

@hxeyexn

저는 개인적으로 style 과 Component 는 별개라고 생각합니다!
CustomView 와 Component 를 비슷한 선상에서 보고 있습니다 😁
Compose 에서도 Theme.kt 에서 ButtonStyle 과 같이 style을 따로 정의해줄 수 있습니다.

(수정) 또 생각해보니 별개는 아니라 생각은 하는데... 흠.. 다시 생각하고 정리해볼게요


저는 Composable 함수를 3가지 관점에서 분리하고 있어요

  1. 기본 컴포넌트

다자인 일관성이 중요한 프로젝트에서만 정의하는데요 (디자이너가 해줘야하지만..)
프로젝트 전역에서 사용하는 기본 컴포넌트이기에 최대한 공통되는 부분만 Composable 내에 정의해두고
세부 사항의 경우에는 외부에서 주입받는 형태로 최대한 변경에 유연하게 대처할 수 있도록 구현합니다.

  1. 조금 더 큰 컴포넌트

이걸 뭐라해야할지 모르겠는데요.. 기본 컴포넌트를 조합한 컴포넌트를 뜻합니다.
만약, 특정 기능 예를 들어 포켓몬 도감 기능에서만 사용하는 컴포넌트의 경우 포켓몬이라는 특성에 초점을 맞춰 설계하는 것 같아요.
기본 컴포넌트보다는 좀 더 세부 구현 사항을 컴포넌트 함수 및 시그니처에 정의해요

  1. 해당 화면에서만 사용하는 컴포넌트

그렇게, 신경써서 설계하지 않습니다 ㅋㅋㅋㅋ 어차피 여기서만 쓰겠지~ 라는 마인드로 StateLess 와 가독성에 초점을 맞춰 설계해요 ㅎㅎ


뭔가 제 컴포넌트 설계 방식을 글로 정리해본적이 없어서... 잘 전달될지 모르겠어요 😅
조금 더 생각해보고 정리해서 다시 전달드릴게욥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants